-
Notifications
You must be signed in to change notification settings - Fork 975
Channel gossip rework #8136
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Channel gossip rework #8136
Conversation
4a98198 to
49de315
Compare
ddaa077 to
af559ce
Compare
af559ce to
68eef71
Compare
68eef71 to
b6db91f
Compare
f40de55 to
ae00452
Compare
endothermicdev
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall. I think we just need to update tests.
| cg->cupdate = tal_free(cg->cupdate); | ||
| send_channel_announcement(channel); | ||
| /* If a channel parameter has changed, send new update */ | ||
| if (update_channel_update(channel, channel_should_enable(channel, true))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will generate a new unsigned channel update for each channel on every new block. Not too heavy for large nodes?
|
|
||
| /* To avoid a storm, we only respond to announcement_signatures with | ||
| * our own signatures once */ | ||
| bool sent_sigs; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's nice to have the announcement signatures loop fixed! It made for some long CI logs.
| const char *description; | ||
| }; | ||
|
|
||
| static struct state_transition allowed_transitions[] = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great! Makes it clear what's happened and easy to follow along as well.
ae00452 to
0df47f1
Compare
Calling the funding tx an anchor is pre-spec terminology, which is now confusing; let's rename the variable. Signed-off-by: Rusty Russell <[email protected]>
This could always happen, but now we're more correct about send it, it's a possible race that I've seen in CI. We can never get a PONG (that's now handled by connectd directly), but we can definitely see this: ``` 2025-05-15T02:29:14.7403081Z # Even with onchain anchor channel, it still keeps reserve (just in case!). 2025-05-15T02:29:14.7403766Z > l1.rpc.close(l2.info['id']) 2025-05-15T02:29:14.7404069Z 2025-05-15T02:29:14.7404248Z tests/test_opening.py:2536: ... 2025-05-15T02:29:14.7413090Z > return self.sock.recv(length) 2025-05-15T02:29:14.7413353Z E Failed: Timeout >1800.0s ... 2025-05-15T02:29:14.8097229Z lightningd-1 2025-05-15T01:50:59.538Z DEBUG 022d223620a359a47ff7f7ac447c85c46c923da53389221a0054c11c1e3ca31d59-closingd-chan#1: billboard perm: Expected closing_signed: 0103fad81b49d5e367cd785e3088b9acf356a7984a17a1ccda86d09da41438840b08000067000001000078c3314666731e339c0b8434f7824797a084ed7ca3655991a672da068e2c44cb53b57b53a296c133bc879109a8931dc31e6913a4bda3d58559b99b95663e6d52679cabaccc6e39dbd95a4dac90e75a258893c3aa3f733d1b8890174d5ddea8003cadffe557773c54d2c07ca1d535c4bf85885f879ae466c16a516e8ffcfec174 ``` Signed-off-by: Rusty Russell <[email protected]>
0df47f1 to
745d408
Compare
It can actually happen, at least in theory. Signed-off-by: Rusty Russell <[email protected]>
In this case a "are we closed onchain". Signed-off-by: Rusty Russell <[email protected]>
We want a more fine-grained approach, so we now have: 1. update_channel_update() - returns true if it changed. 2. channel_should_enable() - should this channel be marked enabled. 3. arm_refresh_timer() - start a refresh timer for the channel_update. Signed-off-by: Rusty Russell <[email protected]>
This message was too verbose (even for trace!) Signed-off-by: Rusty Russell <[email protected]>
They can all call get_block_height(); the extra argument confused me and I thought they were called before the block height was actually updated. Signed-off-by: Rusty Russell <[email protected]>
… wait until 6 deep. The spec used to say you had to wait for channel to be ready, *and* 6 depth before exchanging signatures. Now the 6 depth requirement is only on the actual announcing of the channel: you can send sigs any time. This means our state machine goes from: NOT_USABLE -> NOT_DEEP_ENOUGH -> NEED_PEER_SIGS -> ANNOUNCED to: NOT_USABLE -> NEED_PEER_SIGS -> NOT_DEEP_ENOUGH -> ANNOUNCED However, this revealed that our state machine is insufficient, so rework it to be more general and understandable. In particular, check for unexpected state transitions, and thus document them. Note that cg->sent_sigs replaces channel->replied_to_announcement_sigs, too. Signed-off-by: Rusty Russell <[email protected]> Changelog-Changed: Protocol: We now exchange `announcement_signatures` as soon as we're ready, rather than waiting for 6 blocks (as per recent BOLT update)
745d408 to
2db734b
Compare
(Based on #8135)Merged into masterI tried modifying our state machine to allow us to send (as allowed in the latest spec revisions) announcement_signatures before 6 blocks have passed. However, our state machine did not capture the entire state, so it proved fragile. The result is a rework on the state machine to be more robust.